Skip to content

Conversation

@JohanMabille
Copy link
Collaborator

No description provided.

@@ -0,0 +1,50 @@
name: test-windows
run-name: Tests Windows
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please re-use the test.yaml instead of creating dedicated workflow files. If you want to organize them in individual files, try a design like this.

The main thing is that we should be gating different stages of the CI, e.g. always check a pre-commit first before doing long tests.

Comment on lines +31 to +39
- name: Set conda environment
uses: mamba-org/setup-micromamba@main
with:
environment-name: myenv
environment-file: environment-dev.yml
init-shell: bash
cache-downloads: true
create-args: |
ninja
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please no conda/mamba. If you wish to check it make it a distinct test set. Instead for windows please use vcpkg as the package manager.

Copy link
Collaborator Author

@JohanMabille JohanMabille Oct 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The use of micromamba was to easily get the dependencies, I don't have the need to test it specifically and can use whatever you prefer. Out of curiosity, why are you opposed to using micromamba?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity, why are you opposed to using micromamba?

Mostly that it is the wrong tool in CI. If the purpose is to test the conda ecosystem then it can make sense for that, but then it would be better to have the project packaged there. But for testing OS, using the primary package manager of that OS would be preferred, particularly important being the usage of the native compilers, in this case being msvc.

The use of micromamba was to easily get the dependencies

We are using FetchContent in order to have a package manager independent source of dependencies. Testing the package managers is also quite important since it can have specific patches applied, but in that case only the packaged packages must be used.

Comment on lines 41 to 50
- name: Configure using cmake
run: cmake -Bbuild -DCMAKE_BUILD_TYPE:STING=Release -DCMAKE_INSTALL_PREFIX=$CONDA_PREFIX -G ninja

- name: Build
working-directory: build
run: ninja

- name: Run tests
working-directory: build
run: ctest -R ^test$ --output-on-failure
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Modern approach is to use CMake presets, and I believe this one already has it.

strategy:
fail-fast: true
matrix:
json_version: [ develop, v3.12.0, v3.8.0 ]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be more explicit if you are testing FetchContent dependencies or package manager ones.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants